Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added eq, ne, gt etc. methods. #2175

Merged
merged 8 commits into from
Feb 25, 2022
Merged

Added eq, ne, gt etc. methods. #2175

merged 8 commits into from
Feb 25, 2022

Conversation

Tom1380
Copy link
Contributor

@Tom1380 Tom1380 commented Feb 19, 2022

This is done for convenience and to be friendlier to beginners.

Closes #2155.

@mejrs
Copy link
Member

mejrs commented Feb 20, 2022

Thanks!

Can you add tests for these? 😃

Also, I'm not sure about the names for these. I'm tempted to go with a.eq(b) over a.is_eq(b) and so on. @davidhewitt
since you were the one that suggested these - what do you think?

@birkenfeld
Copy link
Member

Also needs an entry in the changelog, please.

@Tom1380
Copy link
Contributor Author

Tom1380 commented Feb 20, 2022

@mejrs sure, I'd be happy to add tests.

I tried to find where rich_compare itself is tested, but I couldn't find it.
Can you point me to where I can find it? So I can add tests right below it.

@Tom1380 Tom1380 marked this pull request as draft February 20, 2022 20:21
@Tom1380
Copy link
Contributor Author

Tom1380 commented Feb 21, 2022

@mejrs Should I add the tests as documentation tests?

@mejrs
Copy link
Member

mejrs commented Feb 21, 2022

@mejrs Should I add the tests as documentation tests?

I would recommend adding a new test at the bottom of the file where you create a vec with a bunch of different python objects and loop through every possible pair against every compare op, and check whether they give the expected results. That is probably a bit much for a doc example 😉

@davidhewitt
Copy link
Member

davidhewitt commented Feb 21, 2022

Thanks! Regular tests would be better than doctests please; we don't measure coverage for doctests yet.

Also, I'm not sure about the names for these. I'm tempted to go with a.eq(b) over a.is_eq(b) and so on. @davidhewitt
since you were the one that suggested these - what do you think?

The only thing that makes me uneasy about that is that then a.eq(b) clashes with the PartialEq implementation, which uses pointer equality rather than Python equality. I think having the notably different names is a good thing? Or maybe we should consider changing the PartialEq implementation. (Though what to do if __eq__ raises an exception? Panic?)

@mejrs
Copy link
Member

mejrs commented Feb 22, 2022

Oh, that is unfortunate. But having is_eq and eq methods available is also uneasy for me.

In hindsight it may have been better to not have the PartialEq impls and have a ptr_eq method instead.

@birkenfeld
Copy link
Member

Yeah I agree that obj1 == obj2 just comparing pointers is inviting confusion. I'd rather have that exposed as a.is(b) as in Python (ptr_eq is also ok but hard to find when coming from the Python side). Then the way would be clear for eq etc. methods using the richcmp functions, and PartialEq is removed.

The question is, can this have further repercussions than an easy-to-fix compile error? If we're not currently also providing Eq, at least it wasn't possible to put objects into HashMaps.

@davidhewitt
Copy link
Member

I would be happy to remove the current PartialEq implementation. I'm pretty convinced that pointer equality for that is surprising.

@mejrs
Copy link
Member

mejrs commented Feb 22, 2022

I've thought about it some more and I really don't like the PartialEq either.

So, I think the plan is?

  • Remove PartialEq
  • Add PyAny::is(other)
  • Rename all the methods this PR added from is_<op> to just <op>

@davidhewitt
Copy link
Member

davidhewitt commented Feb 22, 2022

We may want to mention the removal in the migration guide, just in case it causes any surprises for users. (Given it's not possible to deprecate a trait implementation as far as I know.)

@birkenfeld
Copy link
Member

I did the PartialEq related stuff in #2183, so this PR just needs some tests and changelog entry to be ready.

@Tom1380
Copy link
Contributor Author

Tom1380 commented Feb 24, 2022

Please let me know if you think the tests are done right, I'm not sure that Rust's cmp logic is the same as Python's.

@Tom1380 Tom1380 changed the title Added is_eq, is_ne, is_gt etc. methods. Added eq, ne, gt etc. methods. Feb 24, 2022
@Tom1380 Tom1380 marked this pull request as ready for review February 25, 2022 03:03
@Tom1380 Tom1380 marked this pull request as draft February 25, 2022 04:53
src/types/any.rs Outdated Show resolved Hide resolved
@birkenfeld
Copy link
Member

I wonder if we shouldn't go one step further here and do the is_true call in the functions.

I know that richcmp methods can return anything, but that is already quite esoteric, and usability here will be much better if only one ? or unwrap is needed.

Those who need the general result can still use rich_compare after all.

@davidhewitt
Copy link
Member

I wonder if we shouldn't go one step further here and do the is_true call in the functions.

I would support that.

@davidhewitt davidhewitt mentioned this pull request Feb 25, 2022
@Tom1380
Copy link
Contributor Author

Tom1380 commented Feb 25, 2022

I wonder if we shouldn't go one step further here and do the is_true call in the functions.

Done.

@Tom1380 Tom1380 marked this pull request as ready for review February 25, 2022 16:05
@Tom1380
Copy link
Contributor Author

Tom1380 commented Feb 25, 2022

I think it's ready. Please review it.

@birkenfeld
Copy link
Member

I think this is good to go. Don't know how the coverage could go down, all methods are tested.

@birkenfeld birkenfeld self-requested a review February 25, 2022 17:51
@birkenfeld birkenfeld merged commit 4873459 into PyO3:main Feb 25, 2022
@davidhewitt
Copy link
Member

Codecov seems to do this weird thing where any PRs with the originating fork branch as main it takes that coverage as our current CI coverage. I noticed this the other day but haven't really had time to investigate or figure out why...

@birkenfeld
Copy link
Member

I also saw that, since rustfmt split the assert as much as possible, it complained about the arguments for the assert error format not being covered. While true, that is unavoidable for working tests :)

@davidhewitt
Copy link
Member

True, yes! Perhaps the upstream macro expansion should consider having the #[no_coverage] attribute added on the assertion failure branch?!

@davidhewitt davidhewitt mentioned this pull request Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_eq, is_gt, etc helper methods
4 participants